This should be in the funding scope to support splicing. Also should probably just be cur_... and prev_... to match existing API pattern rather than a vec that is max-size 2.

Moved to FundingScope as cur/prev fields. TLV 13/15 on FundingScope, 39/41 on the outer scope for backwards compat.

Claude loves to leave comments about how it changed the code that are totally useless - reading the code I don't care what changes claude made in the past, I care if there's something useful to know now. Probably can just drop this.

Removed, and noted for future.

This API doesn't really make sense. There's no information on when I should call this or when the monitor "knows about" a revoked tx. We probably want to do soemthing like the old API where you can fetch revoked transactions in relation to a specific monitor update.

Replaced with sign_initial_justice_tx() for persist_new_channel and sign_justice_txs_from_update(update) for update_persisted_channel. Each is tied to a specific point in the persistence pipeline.

Should we also build justice transactions for all the potential HTLC transactions? I assume we can...

Added. try_sign_justice_txs now iterates nondust_htlcs() on the revoked commitment, builds a spending tx for each HTLC output, and signs with sign_justice_revoked_htlc. Witness follows the same pattern as RevokedHTLCOutput in package.rs. Dust HTLCs are skipped when fee exceeds value.

Also added test_justice_tx_htlc_from_monitor_updates which routes a payment, captures the commitment with a pending HTLC, revokes it, and verifies the watchtower gets justice txs for both to_local and the HTLC output.

I assume we can/should do this?

Done.

Bug: sign_justice_txs_from_update mutates the monitor's in-memory state via .take() on prev_counterparty_commitment_tx. Since this is called inside a Persist callback, if the persist operation fails and is retried, the prev commitment has already been removed from memory — the retry will produce no justice txs.

The mutation is committed immediately (interior mutability via Mutex), but the serialized/persisted state may not reflect it if the persist fails. On a process restart, the old persisted state would have the prev, but during the same process lifetime, the data is gone.

Consider either:

• Making this idempotent (don't take, just check and sign — dedup on the caller side)
• Or cloning instead of taking, and only clearing after confirmed persist

The rotation prev = cur.take(); cur = new; happens before checking whether the old prev had a revocation secret available. If the caller skips calling sign_justice_txs_from_update for an update that only contains a revocation (no new commitment), and then calls it for the next update that does contain a new commitment, the old prev is overwritten without ever being signed.

This is safe under the assumption that the caller invokes this for every ChannelMonitorUpdate, but the doc comment doesn't state this requirement. The method should either:

1. Document that it MUST be called for every update (not just commitment-bearing ones), or
2. Check the old prev for revocation secrets before the rotation, sign it if ready, and then rotate.

This unconditionally overwrites cur_counterparty_commitment_tx with the initial commitment. Since provide_initial_counterparty_commitment_tx (line 3539) already sets this field, this is redundant in the normal path.

More importantly, since sign_initial_justice_txs is a pub method, a caller who mistakenly invokes it after updates have been applied would reset cur_counterparty_commitment_tx back to the initial commitment, silently corrupting the rotation state for sign_justice_txs_from_update. Consider guarding this (e.g., only set if cur_counterparty_commitment_tx.is_none()).

debug_assert!(false) only fires in debug builds. In release, the new commitment is silently dropped, meaning the watchtower will miss this commitment entirely (and any subsequent ones for this funding scope). At minimum, this should log a warning. Consider log_error! or returning an error.

Nit: This constructs a full Transaction object (cloning input and destination_script) purely for weight estimation. The existing build_to_local_justice_tx does this too, but here it happens in a loop for every HTLC. Consider computing the base weight once outside the loop since it's the same for all HTLC justice txs (single input, single output to the same destination_script).

These fields are added to FundingScope's impl_writeable_tlv_based! (at TLV 13/15, which applies to pending_funding entries), and to the top-level write_chanmon_internal (at TLV 39/41, for the main funding scope). During deserialization, the main funding scope reads from TLV 39/41 while pending_funding entries read from their per-FundingScope TLV 13/15.

This split serialization works but is subtle and fragile — a future change that serializes the main funding as a whole FundingScope would silently double-write the data. Worth adding a comment explaining why these live in two places.

The old justice_tx method returned the single justice tx directly. Now it returns only the first element from the vec via .first(). This works because try_sign_justice_txs pushes the to_local justice tx first, but this ordering is an implicit invariant. If the order in try_sign_justice_txs ever changes, existing callers of justice_tx would silently get the wrong transaction. Consider adding a comment documenting this assumption, or filtering by output type.

This needs a lot more detail. One big concern is that we need to describe how crash-safety works. Let's say I start storing a ChannelMonitorUpdate while I'm waiting on my watchtower to persist my new transactions, but then crash before the second. I have to make sure the transactions get reloaded on restart somehow and aren't lost, even if the ChannelMonitorUpdate made it to disk (and thus wasn't replayed). Not sure if this should be handled by forcing downstream logic to persist before the monitor or if we should have some way to do a replay.

You're right, the update-already-persisted case means replay alone isn't enough.

I think the cleanest fix is: keep the prev/cur commitment data in the monitor (clone instead of .take()), serialize it, and add a get_pending_justice_txs() method that reads the stored state without mutating it. On restart, the Persist impl calls this against the loaded monitor to recover any justice txs it didn't finish sending.

The full crash-safety model would be:

• Normal path: Persist impl gets justice txs from the update, sends to watchtower, returns Completed
• Crash recovery: Persist impl calls get_pending_justice_txs() on the loaded monitor at startup, re-sends anything the watchtower doesn't have
• The watchtower deduplicates on its end (or the Persist impl tracks what's been acked)

This makes the API idempotent and crash-safe without requiring any ordering constraints on when the monitor update is persisted. I'll rework the implementation and expand the docs.

I think that seems reasonable. My question then is do we even need sign_justice_txs_from_update? Should we instead drop the from-update API and replace it with a simple bool on a ChannelMonitorUpdate to check if there is a commitment update that may need to be written to a watchtower?

Agreed. Dropped both sign_justice_txs_from_update and sign_initial_justice_txs. The API is now just get_pending_justice_txs() plus ChannelMonitorUpdate::updates_watchtower_state() to signal when to call it. Rotation happens in the normal update path.

Bug: This doc claim is false — the method is not idempotent within a single process lifetime.

While the signing phase clones prev (line 4914), the rotation phase at lines 4888-4889 still uses cur_counterparty_commitment_tx.take(), which consumes cur. On a second call with the same update:

1. counterparty_commitment_txs_from_update re-extracts the same new commitment tx
2. The funding scope match succeeds (same txid)
3. prev = cur.take() — but cur now holds the new commitment (set by the first call), so prev gets overwritten with the new commitment, discarding the old revoked commitment
4. Secret matching against prev fails (new commitment number ≠ revocation index)
5. No justice transactions are produced

This matters if a Persist implementation retries on transient failure (e.g., network error to a remote store) without restarting the process. The watchtower would silently miss revoked commitments.

Suggest either fixing the rotation to not use .take() (e.g., clone cur into prev), or correcting this doc to state that the method must only be called once per update per process lifetime.

Bug: The rotation and signing are ordered incorrectly for the case where a single ChannelMonitorUpdate contains both a new counterparty commitment step and a CommitmentSecret step that revokes the commitment that was in prev before the rotation.

Example: before this method runs, cur = C(n), prev = C(n+1). An update arrives containing LatestCounterpartyCommitmentTXInfo(C(n-1)) and CommitmentSecret(idx = n+1).

1. Rotation: prev = C(n) (was cur), cur = C(n-1) — C(n+1) is lost
2. new_secret_indices = [n+1]
3. Check prev.commitment_number() = n — doesn't match n+1
4. Justice tx for C(n+1) is never produced

The fix is to check/sign the old prev against available secrets before performing the rotation. Whether this scenario actually occurs in current LDK channel code depends on whether commitment_signed and revoke_and_ack processing ever produce a single batched ChannelMonitorUpdate — but the method's contract doesn't prohibit it, and future code changes could introduce this pattern.

Nit: This assertion is extremely weak — commitment_number < INITIAL_COMMITMENT_NUMBER is true for any non-initial commitment and would pass even if get_pending_justice_txs returned a completely unrelated commitment.

Since only one rotation occurs in this test, consider asserting that pending[0].revoked_commitment_txid == revoked_txid, which would actually validate the crash recovery round-trip rather than trivially passing.

Ha, I don't think we need to document things that are impossible :)

Removed :)

I think that seems reasonable. My question then is do we even need sign_justice_txs_from_update? Should we instead drop the from-update API and replace it with a simple bool on a ChannelMonitorUpdate to check if there is a commitment update that may need to be written to a watchtower?

Hmm? This doesn't "store" anything?

Removed the method. Storage now happens during update_counterparty_commitment_data and the LatestCounterpartyCommitmentTXInfo processing path.

In practice we could restart and the counterparty could give us a different commitment transaction. They shouldn't, but they could. I imagine this implies we should instead only send the justice txn to the watchtower after persistence?

Makes sense. The WatchtowerPersister example now calls get_pending_justice_txs after persistence completes, not before.

Okay but what if we lose connectivity to the watchtower but not our peer? We'd then step the channel forward twice and on restart lose the previous state. ISTM we have to tell clients to hold on to monitor updates if there's still a pending watchtower write for a previous update.

Agreed. Added doc guidance that the Persist implementation should delay completing monitor updates until previously obtained justice transactions have been delivered.

Bug: get_pending_justice_txs is idempotent by design — it returns the same results on repeated calls for the same monitor state. But this code uses Vec::push, which accumulates duplicates across consecutive watchtower-relevant updates that don't rotate prev.

Example: a CommitmentSecret update followed by a RenegotiatedFunding update — both trigger updates_watchtower_state(), and prev doesn't change between them, so the same justice tx(s) are pushed into the Vec twice.

The old code used HashMap::insert(commitment_txid, signed_justice_tx) which was an idempotent upsert. Consider either:

• Using entry().or_insert() instead of push (replacing rather than appending when the txid already has entries)
• Or clearing the Vec for this txid before pushing the new results

Note: this replaces rather than appends, which is correct since get_pending_justice_txs always returns the complete set for each revoked commitment.

The comment says "for the LatestCounterpartyCommitmentTXInfo path" but this post-loop is also the only mechanism that sets cur_counterparty_commitment_tx for RenegotiatedFunding scopes (created at line 4097 with cur_counterparty_commitment_tx: None).

If this code is removed during the planned LatestCounterpartyCommitmentTXInfo deprecation, RenegotiatedFunding (splice) scopes would silently lose their initial commitment tracking — cur would stay None, so after the next commitment update, prev would be None instead of the initial splice commitment, and the watchtower would miss that revocation.

Suggest updating the comment:

The fee estimation constructs a throwaway Transaction with input.clone() and destination_script.clone() per HTLC, but the base tx weight (excluding witness) is identical for every HTLC justice tx in this loop — same version, locktime, single input (empty witness), single output to the same destination script.

Consider hoisting the base weight calculation before the loop and only varying the witness weight per HTLC type:

let base_weight = Transaction {
    version: Version::TWO,
    lock_time: LockTime::ZERO,
    input: vec![TxIn { ..Default::default() }],
    output: vec![TxOut { script_pubkey: destination_script.clone(), value: Amount::ZERO }],
}.weight().to_wu();

// Then inside the loop:
let fee = fee_for_weight(feerate_per_kw as u32, base_weight + weight_estimate);

This avoids cloning input and destination_script on every iteration. (Reiterating prior comment #5 with a concrete suggestion since it's in a per-HTLC loop that could be large.)

This test is named "crash_recovery" but doesn't actually test the crash recovery path — it never serializes, discards in-memory state, and deserializes the monitor. It just checks that get_pending_justice_txs returns results during normal (non-crash) operation, which is already covered by test_justice_tx_idempotent.

A meaningful crash recovery test would:

1. Serialize the monitor after the revocation
2. Deserialize it into a fresh ChannelMonitor
3. Call get_pending_justice_txs on the deserialized monitor
4. Verify the justice tx is present and valid

This would exercise the TLV 43/45 serialization round-trip and confirm that prev_counterparty_commitment_tx survives persistence — the actual invariant that crash recovery depends on.

Bug (security-critical): Guard condition causes permanent loss of revoked commitments after the first channel state update.

This guard checks if prev already holds a revoked commitment (secret available) and, if so, refuses to rotate. The problem is this check is permanently sticky — once a secret is provided, get_secret() returns Some forever. After the first CommitmentSecret update revokes the commitment in prev, every subsequent LatestCounterpartyCommitment update will hit this guard, and the outgoing cur (which becomes revoked in the next CommitmentSecret update) is silently dropped.

Trace with 4 sequential updates (normal commitment_signed/revoke_and_ack flow):

At Update C, C(N-1) was in cur and is silently overwritten. At Update D, its secret arrives but C(N-1) is in neither prev nor cur. The watchtower never gets the justice tx for C(N-1).

This repeats for every subsequent commitment — only C(N) (the very first revoked commitment) is ever returned. All later revoked commitments are lost.

Fix: Remove the guard entirely. Without it, the rotation prev = cur.take(); cur = new; works correctly: each commitment spends exactly one update cycle in prev, and by the time its secret arrives (in the next CommitmentSecret update), it's still in prev.

The same bug exists at lines 3654-3658 for pending_funding scopes.

Nit: Inconsistent indentation — line 4395 has an extra leading tab compared to line 4396. Both should be at the same indentation level within the if block.

Bug: Lines 1139-1140 are identical — assert!(!pending.is_empty()) is duplicated. This looks like a copy-paste error. The second assertion should check something meaningful, e.g.:

This would actually validate that get_pending_justice_txs returns the justice tx for the correct revoked commitment, rather than just checking non-emptiness twice.